-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issues/247 Remove Seperation of Datapath and Filename in Requests #339
Issues/247 Remove Seperation of Datapath and Filename in Requests #339
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good.
It might make sense to rebase f563dab out and save it for the PR that actually unifies uploaded and mounted data in the UI.
src/components/TrackFilePicker.js
Outdated
<div data-testid={testID}> | ||
<Input | ||
type="file" | ||
className="customDataUpload form-control-file" | ||
accept={acceptedExtensions} | ||
innerRef={uploadFileInput} | ||
onChange={uploadOnChange} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part doesn't actually work yet in this branch, right? But also it's not displayed yet?
src/config.json
Outdated
@@ -4,19 +4,19 @@ | |||
{ | |||
"name": "snp1kg-BRCA1", | |||
"tracks": [ | |||
{"trackFile": "snp1kg-BRCA1.vg.xg", "trackType": "graph", "trackColorSettings": {"mainPalette": "greys", "auxPalette": "ygreys"}}, | |||
{"trackFile": "NA12878-BRCA1.sorted.gam", "trackType": "read"} | |||
{"trackFile": "./exampleData/internal/snp1kg-BRCA1.vg.xg", "trackType": "graph", "trackColorSettings": {"mainPalette": "greys", "auxPalette": "ygreys"}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the leading ./
es are redundant; I think exampleData/whatever
ought to work fine.
src/server.mjs
Outdated
fs.readdirSync(MOUNTED_DATA_PATH).forEach((file) => { | ||
if (endsWithExtensions(file, GRAPH_EXTENSIONS)) { | ||
result.files.push({"trackFile": file, "trackType": "graph"}); | ||
result.files.push({"trackFile": path.resolve(config.dataPath, file), "trackType": "graph"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're looping over the contents of MOUNTED_DATA_PATH
, but resolving the absolute paths with config.dataPath
. Those should really both be the same expression to ensure we actually end up pointing to the files we are looking at.
We might want to have a const absPath = path.resolve(config.dataPath, file)
outside all the conditionals, also. Instead of repeating it so much.
src/components/TrackFilePicker.js
Outdated
const fileTypeToExtensions = { | ||
"graph": ".xg,.vg,.hg,.gbz,.pg", | ||
"read": ".gbwt,.gbz", | ||
"haplotype": ".gam", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think read
and haplotype
are swapped here.
Can we make these come from the same constants used on the server, like HAPLOTYPE_EXTENSIONS
, by moving the strings to a common file?
@@ -310,20 +307,20 @@ class HeaderForm extends Component { | |||
} else { | |||
json.bedFiles.unshift("none"); | |||
|
|||
if (this.state.dataPath === "mounted") { | |||
if (this.state.dataType === dataTypes.MOUNTED_FILES) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename MOUNTED_FILES
to maybe CUSTOM
?
2cbe166
to
5df07fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good!
We might have wanted to update the doc comments for the BedFileDropdown to reflect its newly specialized purpose and the fancy filename stuff it now does.
No description provided.